Skip to content

fix: type import from csstype#1457

Merged
kof merged 2 commits intocssinjs:masterfrom
fardad-dev:patch-1
Feb 25, 2021
Merged

fix: type import from csstype#1457
kof merged 2 commits intocssinjs:masterfrom
fardad-dev:patch-1

Conversation

@fardad-dev
Copy link
Copy Markdown
Member

@fardad-dev fardad-dev commented Feb 21, 2021

Corresponding issue (if exists):

Typescript not working I changed import from csstype and now that working

What would you like to add/fix?

Todo

  • Add test that verifies the modified behavior
  • Add documentation if it changes public API

@fardad-dev fardad-dev requested a review from kof as a code owner February 21, 2021 14:37
@kof kof requested a review from moshest February 22, 2021 09:24
type NormalCssProperties = CSSProperties<string | number>
type NormalCssValues<K> = K extends keyof NormalCssProperties
? NormalCssProperties[K] | JssValue
? NormalCssProperties[K]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems as a breaking change.. why removing it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JssValue in there converting NormalCssValues to any.

I think that not needed because when the key is a key of NormalCssProperties we should respond type of that key

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but this can be a breaking change for some users. If that not required I prefer to remove it and maybe push it on a separate PR as breaking change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have used jss in huge typescript project, there is nothing broke.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, if someone use a type that's not defined on the standard properties from some reason it will break. No sure if this consider a break or a fix, but I remember some complains in the past about this. NormalCssProperties is not perfect for all properties.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked styled components, they do show an error when an unknown property is used

Screenshot 2021-02-23 at 10 52 16

What about '& .ant-select-selector'?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably use for this the new TS feature and identify nesting by "&"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, you can merge this PR, and we can continue to discuss in an issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moshest seems like we can merge it since because of flexible JssValue it should be breaking it right now, but please check it out, I have no code base in TS using it to do so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have time to test it right now but I'm ok with merging it. It should be fine for most users anyway..

@kof kof merged commit ca8269f into cssinjs:master Feb 25, 2021
@kof
Copy link
Copy Markdown
Member

kof commented Feb 25, 2021

merged! thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants